Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support setting per-role config with ALTER ROLE SET #4

Merged
merged 3 commits into from
Oct 6, 2022
Merged

Support setting per-role config with ALTER ROLE SET #4

merged 3 commits into from
Oct 6, 2022

Conversation

jfleury-gorgias
Copy link

@jfleury-gorgias jfleury-gorgias commented Sep 30, 2022 •
edited
Loading

Upstream PR: Squarespace/pgbedrock#73

⚠️ With this change, pgbedrock becomes the authority on user config and will override any existing config (or reset them if they are not set in the configuration).

@jfleury-gorgias jfleury-gorgias requested a review from a team September 30, 2022 15:23
@linear
Copy link

linear bot commented Sep 30, 2022

PLTSR-1003 Patch pgbedrock to support ALTER ROLES SET

This way we can use it to set per-role parameters like timeout.

Squarespace#65

@jfleury-gorgias
Copy link
Author

I still need to make sure this works as expected on staging.

@jfleury-gorgias jfleury-gorgias force-pushed the jfleury/pltsr-1003-patch-pgbedrock-to-support-alter-roles branch from fd0088f to 3b27a95 Compare October 4, 2022 19:09
@jfleury-gorgias
Copy link
Author

So this was successfully tested on staging.

Here are the logs from pgbedrock:

--------------------------------
--- Configuring attributes -----
--------------------------------

ALTER ROLE "gorgias" SET statement_timeout="10min"; -- Previous value: 5min
ALTER ROLE "gorgias" RESET idle_in_transaction_session_timeout; -- Previous value: 2min

And how the settings have been changed (from SELECT usename,useconfig FROM pg_shadow WHERE useconfig IS NOT NULL ORDER BY usename;):

     usename     |                            useconfig
 ----------------+------------------------------------------------------------------
  debezium       | {idle_in_transaction_session_timeout=2min}
- gorgias        | {statement_timeout=5min,idle_in_transaction_session_timeout=2min}
+ gorgias        | {statement_timeout=10min}
  gorgias_web    | {statement_timeout=55s,gin_pending_list_limit=512kB}
  gorgias_worker | {statement_timeout=55s,gin_pending_list_limit=512kB}
  periscope      | {statement_timeout=55s,idle_in_transaction_session_timeout=2min}

@jfleury-gorgias jfleury-gorgias marked this pull request as ready for review October 4, 2022 19:10
@jfleury-gorgias
Copy link
Author

I created gorgias/gorgias#12995 if you want to see the change in the config.

FlipEnergy
FlipEnergy previously approved these changes Oct 4, 2022
View reviewed changes
Copy link
Member

@FlipEnergy FlipEnergy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good work, Johan. LGTM.

@jfleury-gorgias jfleury-gorgias force-pushed the jfleury/pltsr-1003-patch-pgbedrock-to-support-alter-roles branch from 3b27a95 to 8316248 Compare October 5, 2022 18:36
@jfleury-gorgias
Copy link
Author

@FlipEnergy I just imported your commit from the upstream PR.

deadlytea
deadlytea reviewed Oct 6, 2022
View reviewed changes
Copy link

@deadlytea deadlytea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have some tests for the byte/duration parse logic before we put this in, also I'm not a fan of configs as the wording choice as it makes me think of a set of config structures, not a set of parameters, but I won't block on that we can update if we can think of something better

@jfleury-gorgias jfleury-gorgias force-pushed the jfleury/pltsr-1003-patch-pgbedrock-to-support-alter-roles branch from 8316248 to 2531cda Compare October 6, 2022 16:43
@jfleury-gorgias
Copy link
Author

jfleury-gorgias commented Oct 6, 2022 •
edited
Loading

Would be nice to have some tests for the byte/duration parse logic before we put this in

Done and it helped me find some bugs, I should’ve done that earlier.

deadlytea
deadlytea approved these changes Oct 6, 2022
View reviewed changes
Copy link

@deadlytea deadlytea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jfleury-gorgias jfleury-gorgias merged commit c539468 into master Oct 6, 2022
@jfleury-gorgias jfleury-gorgias deleted the jfleury/pltsr-1003-patch-pgbedrock-to-support-alter-roles branch October 6, 2022 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deadlytea deadlytea deadlytea approved these changes

@FlipEnergy FlipEnergy FlipEnergy left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jfleury-gorgias @deadlytea @FlipEnergy @johanfleury